Integration tests #148

QuentinI · 2024-09-30T12:00:59Z

This PR:

Adds an integration testing "framework" based on HotShot test harness.
Integration tests for builder are based on implementing TestBuilderImplementation, which allows running HotShot test harness with custom builder implementation. Additionally, this PR implements two TestTasks that can be injected into HotShot harness:

TransactionGenerationTask, which allows for more flexible transaction generation than what's provided in HotShot harness
BuilderValidationTask, which applies custom validation logic to verify builder-related properties of the chain. Currently it only verifies transaction ordering and a transaction count threshold, but can be extended to verify other properties as needed, e.g. latency.

This PR does not:

Planned, but not included: integration testing for legacy builder: best done after BuilderState refactor to re-use most of the code from marketplace builder
Add a significant amount of tests: some basic tests are present in crates/marketplace/src/testing/integration.rs

Key places to review:

crates/marketplace/src/testing/integration.rs

coveralls · 2024-10-17T19:32:41Z

Pull Request Test Coverage Report for Build 11481521970

Details

1 of 318 (0.31%) changed or added relevant lines in 7 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-7.3%) to 71.937%

Changes Missing Coverage	Changed/Added Lines	%
crates/shared/src/utils.rs	2	0.0%
crates/legacy/src/service.rs	3	0.0%
crates/shared/src/testing/mod.rs	37	0.0%
crates/shared/src/testing/validation.rs	51	0.0%
crates/marketplace/src/testing/integration.rs	100	0.0%
crates/shared/src/testing/generation.rs	124	0.0%

Totals
Change from base Build 11480896880:	-7.3%
Covered Lines:	2384
Relevant Lines:	3314

💛 - Coveralls

dailinsubjam

Would appreciate if we could have a description about this PR. More comments in crates/marketplace/src/testing/integration.rs would also be helpful for review. Thanks!

QuentinI · 2024-10-22T12:39:45Z

@dailinsubjam done, hopefully description + additional comments will be helpful. Please request more docs if not, there's no such thing as enough docs 🙂

shenkeyao · 2024-10-22T23:18:51Z

crates/shared/src/testing/validation.rs

+{
+    type Event = Event<Types>;
+
+    async fn handle_event(&mut self, (event, id): (Self::Event, usize)) -> anyhow::Result<()> {


Didn't know we could define names like (event, id) here!

You can do basically any patterns in function arguments!
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=712d93ebe3c92450a0d9a0483005c147

shenkeyao · 2024-10-22T23:19:34Z

crates/shared/src/testing/validation.rs

+        if id != 0 {
+            return Ok(());
+        }


Could you add a comment about what id is and why we can return Ok if it's nonzero?

shenkeyao · 2024-10-22T23:30:29Z

crates/shared/src/testing/validation.rs

+        if let Err(e) = self
+            .txn_history
+            .iter()
+            .map(|txn| txn.payload.number as i64)


Looking at the code here I feel like index is probably a better field name than number, to distinguish it from "count".

shenkeyao · 2024-10-23T00:00:07Z

crates/shared/src/testing/generation.rs

+        if id != 0 {
+            return Ok(());
+        }


Same here, can you add a comment about id and why we return Ok here?

shenkeyao · 2024-10-23T00:01:35Z

crates/shared/src/testing/generation.rs

+                                // If the builder returns an error, we will re-try on the next view
+                                error => {
+                                    tracing::warn!(?error, "Builder API error");
+                                    self.txn_queue.push_front(txn);


Why do we push to txn_queue if the submission fails?

To try again next view, so that transactions we submit to the builder are numbered sequentially, otherwise we wouldn't be able to determine whether ordering failed because something's wrong with the builder or because one of transactions was rejected by the builder

shenkeyao · 2024-10-23T00:04:23Z

crates/marketplace/src/testing/integration.rs

+        n_nodes: usize,
+        url: Url,
+        config: Self::Config,
+        _changes: HashMap<u64, BuilderChange>,


Is this argument for future tests?

It is required by the trait, in HotShot it is used to test how network behaves when some of the builders go offline / restart / start failing responses. As it mostly used to test proposer behavior rather than the builder's, I decided to ignore it here, although maybe we'll want to make use of it in some more complex tests

dailinsubjam · 2024-10-23T18:48:27Z

crates/marketplace/src/testing/integration.rs

+    fn start(
+        self: Box<Self>,
+        stream: Box<
+            dyn futures::prelude::Stream<Item = hotshot::types::Event<Types>>


Looks like start() in implementation of trait TestBuilderImplementation is for starting [ProxyGlobalState] APIs and initial [BuilderState]'s event loop (as the comment said). Then what's this start() for? What's the difference between run_builder_service() and starting the event loop?

run_builder_service starts a task that takes a hotshot event stream and processes events from it (applies hooks and forwards to builder states)
The naming is rather non-intuitive I think, something to address during the upcoming refactor.

dailinsubjam · 2024-10-23T19:03:21Z

crates/shared/src/testing/generation.rs

+/// - `Random` submits a certain amount of random transactions every view
+/// - `Seeded` accepts a map of transactions to submit on each view
+/// - `Flood` continually submits transactions, re-trying if it encounters an error on


dailinsubjam · 2024-10-23T19:24:34Z

crates/shared/src/testing/validation.rs

+            .iter()
+            .map(|txn| txn.payload.index as i64)
+            .try_fold(-1, |prev, next| {
+                if prev + 1 == next {


Do we allow silent drop here? If so we might want to change to prev < next

I was writing this before we even figured out silent drop exists, but I'd leave it as is nonetheless because silent drop is extremely unlikely to happen in integration tests because all nodes behave and network is as good as it gets, so we won't trigger the silent drop accidentally. So I'd leave this check as-is because failure here would be very likely indication of another bug in builder rather than silent drop issue.
Then, as part of fixing the silent drop, we can specifically engineer a test where conditions for it happen in a reproducible manner.

QuentinI marked this pull request as draft September 30, 2024 12:35

ss-es reviewed Sep 30, 2024

View reviewed changes

crates/marketplace/src/testing/integration.rs Outdated Show resolved Hide resolved

QuentinI force-pushed the ag/integration branch 2 times, most recently from 9d274d2 to f502edd Compare October 10, 2024 16:38

QuentinI added 4 commits October 11, 2024 14:30

Extract shared crate

0b86c7a

impl Stream -> generic param

69579c9

Merge remote-tracking branch 'origin/main' into ag/shared-crate

f327230

Merge branch 'main' into ag/shared-crate

a3ac38c

QuentinI force-pushed the ag/integration branch from 39f7317 to cc78cd5 Compare October 16, 2024 12:32

QuentinI changed the base branch from main to ag/shared-crate October 16, 2024 12:33

Integration tests: WIP

cdea9e1

QuentinI force-pushed the ag/integration branch from cc78cd5 to cdea9e1 Compare October 16, 2024 12:34

QuentinI added 4 commits October 16, 2024 14:36

Run CI on all PRs

37eeb24

Fix lint

6f15c31

Add stress test

247b35d

Point to HotShot main

4d37a3c

Base automatically changed from ag/shared-crate to main October 16, 2024 15:24

QuentinI added 2 commits October 16, 2024 17:28

Merge remote-tracking branch 'origin/main' into ag/integration

7b2c3a6

Clearer naming in utils

01c39f6

QuentinI marked this pull request as ready for review October 16, 2024 17:08

QuentinI changed the title ~~[WIP] integration tests~~ Integration tests Oct 16, 2024

Stress less

51e9ab5

QuentinI force-pushed the ag/integration branch from 53f6751 to 51e9ab5 Compare October 16, 2024 19:58

Merge remote-tracking branch 'origin/main' into ag/integration

3582cf4

QuentinI added 5 commits October 18, 2024 18:29

Merge remote-tracking branch 'origin/main' into ag/integration

87e6fb7

Merge remote-tracking branch 'origin/main' into ag/integration

4066eee

Merge remote-tracking branch 'origin/main' into ag/integration

c58914e

Merge remote-tracking branch 'origin/main' into ag/integration

afb4cea

Merge remote-tracking branch 'origin/main' into ag/integration

f479bd0

dailinsubjam reviewed Oct 21, 2024

View reviewed changes

Add documentation

2b1c75c

shenkeyao reviewed Oct 23, 2024

View reviewed changes

QuentinI added 2 commits October 23, 2024 15:29

Comments

2409eb2

Comments x2

1077625

QuentinI force-pushed the ag/integration branch from 56b2610 to 1077625 Compare October 23, 2024 14:00

shenkeyao approved these changes Oct 23, 2024

View reviewed changes

dailinsubjam reviewed Oct 23, 2024

View reviewed changes

dailinsubjam approved these changes Oct 24, 2024

View reviewed changes

QuentinI merged commit 87c19be into main Oct 24, 2024
7 checks passed

QuentinI deleted the ag/integration branch October 24, 2024 15:57

QuentinI mentioned this pull request Oct 28, 2024

[Test Improvement] - Add Integration Framework #208

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration tests #148

Integration tests #148

QuentinI commented Sep 30, 2024 •

edited

Loading

coveralls commented Oct 17, 2024 •

edited

Loading

dailinsubjam left a comment

QuentinI commented Oct 22, 2024 •

edited

Loading

shenkeyao Oct 22, 2024

QuentinI Oct 23, 2024 •

edited

Loading

shenkeyao Oct 22, 2024

shenkeyao Oct 22, 2024

shenkeyao Oct 23, 2024

shenkeyao Oct 23, 2024

QuentinI Oct 23, 2024

shenkeyao Oct 23, 2024

QuentinI Oct 23, 2024

dailinsubjam Oct 23, 2024 •

edited

Loading

QuentinI Oct 24, 2024

dailinsubjam Oct 23, 2024

dailinsubjam Oct 23, 2024

QuentinI Oct 24, 2024

Integration tests #148

Integration tests #148

Conversation

QuentinI commented Sep 30, 2024 • edited Loading

This PR:

This PR does not:

Key places to review:

coveralls commented Oct 17, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11481521970

Details

💛 - Coveralls

dailinsubjam left a comment

Choose a reason for hiding this comment

QuentinI commented Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

QuentinI Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dailinsubjam Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuentinI commented Sep 30, 2024 •

edited

Loading

coveralls commented Oct 17, 2024 •

edited

Loading

QuentinI commented Oct 22, 2024 •

edited

Loading

QuentinI Oct 23, 2024 •

edited

Loading

dailinsubjam Oct 23, 2024 •

edited

Loading